-
Notifications
You must be signed in to change notification settings - Fork 21
Pm 959 tc finance integration #1075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…x-and-fees PM-1222 - show taxes & fees applied to payments
|
|
||
| import { Balance, WalletDetails } from '../../../lib/models/WalletDetails' | ||
| import { getWalletDetails } from '../../../lib/services/wallet' | ||
| import { Balance } from '../../../lib/models/WalletDetails' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement for WalletDetails has been removed, but it is unclear if this is intentional or if WalletDetails is still needed elsewhere in the code. Please verify if WalletDetails is used in other parts of the file or project.
| import { Balance } from '../../../lib/models/WalletDetails' | ||
| import { InfoRow } from '../../../lib' | ||
| import { BannerImage, BannerText } from '../../../lib/assets/home' | ||
| import { nullToZero } from '../../../lib/util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider checking if nullToZero is used in the file. If not, ensure that its import is necessary or remove it to keep the code clean.
| const HomeTab: FC<HomeTabProps> = () => { | ||
| const [walletDetails, setWalletDetails] = useState<WalletDetails | undefined>(undefined) | ||
| const [isLoading, setIsLoading] = useState(false) | ||
| const { data: walletDetails, isLoading, error }: WalletDetailsResponse = useWalletDetails() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling the error state more explicitly. Currently, the error variable is being destructured from useWalletDetails(), but there is no logic to handle or display this error within the component. Implementing error handling will improve the user experience by providing feedback when the wallet details cannot be fetched.
| const [walletDetails, setWalletDetails] = useState<WalletDetails | undefined>(undefined) | ||
| const [isLoading, setIsLoading] = useState(false) | ||
| const { data: walletDetails, isLoading, error }: WalletDetailsResponse = useWalletDetails() | ||
| const [balanceSum, setBalanceSum] = useState(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The balanceSum state is initialized but not used in the provided code snippet. Ensure that this state is necessary, and if it is, implement logic to update and utilize it. If not, consider removing it to clean up the code.
| </div> | ||
| {isLoading && <LoadingCircles />} | ||
| {!isLoading && ( | ||
| {!isLoading && walletDetails && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a null check for walletDetails to ensure it is not null or undefined before rendering the component. This will prevent potential runtime errors if walletDetails is not properly initialized.
| } | ||
| /> | ||
|
|
||
| {walletDetails.withdrawalMethod.isSetupComplete && walletDetails.taxForm.isSetupComplete && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a check for walletDetails before accessing its properties to prevent potential runtime errors if walletDetails is undefined.
| <InfoRow | ||
| title='Est. Payment Fees and Tax Withholding %' | ||
| // eslint-disable-next-line max-len | ||
| value={`Fee: ${nullToZero(walletDetails.estimatedFees)} USD / Tax Withholding: ${nullToZero(walletDetails.taxWithholdingPercentage)}%`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment // eslint-disable-next-line max-len suggests that the line exceeds the maximum length limit. Consider refactoring the line to adhere to the length limit for better readability and maintainability.
| icon={IconOutline.ArrowRightIcon} | ||
| size='md' | ||
| link | ||
| to='#payout' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The to prop value '#payout' seems to be a hash link. Ensure that the corresponding element with the id payout exists in the DOM to avoid broken links.
| line-height: 20px; | ||
| font-size: 14px; | ||
|
|
||
| + .taxes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more descriptive class name instead of .mt to improve readability and maintainability of the code.
| /* eslint-disable react/jsx-no-bind */ | ||
| import { toast } from 'react-toastify' | ||
| import { AxiosError } from 'axios' | ||
| import { Link } from 'react-router-dom' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Link import from react-router-dom is added but not used in the code. Consider removing it if it's not needed to avoid unnecessary imports.
| import { ConfirmFlowData } from '../../../lib/models/ConfirmFlowData' | ||
| import { PaginationInfo } from '../../../lib/models/PaginationInfo' | ||
| import { useWalletDetails, WalletDetailsResponse } from '../../../lib/hooks/use-wallet-details' | ||
| import { nullToZero } from '../../../lib/util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function nullToZero is imported but not used in this file. Consider removing the import if it's not needed.
| fetchWinnings() | ||
| } | ||
|
|
||
| function handlePayMeClick( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming handlePayMeClick to a more descriptive name like handlePaymentConfirmationClick to better convey the function's purpose.
|
|
||
| function handlePayMeClick( | ||
| paymentIds: { [paymentId: string]: Winning }, | ||
| totalAmount: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The totalAmount parameter is typed as a string. Consider using a numeric type if this value is intended to represent a monetary amount to avoid potential issues with numeric operations.
| <div className={`${styles.taxes} ${styles.mt}`}> | ||
| You can adjust your payout settings to customize your estimated payment fee and tax withholding percentage in | ||
| {' '} | ||
| <Link to='#payout'>Payout</Link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Link component's to prop is set to '#payout'. Ensure that this hash link correctly corresponds to an existing element ID on the page to avoid broken links.
| title: 'Are you sure?', | ||
| }) | ||
| }} | ||
| onPayMeClick={handlePayMeClick} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function handlePayMeClick is used here, but its definition and implementation are not visible in the diff. Ensure that handlePayMeClick correctly replicates the functionality of the previous inline function, including setting the confirmFlow state and calling processPayouts with the correct parameters.
| export interface Response<T> { | ||
| data?: Readonly<T> | ||
| error?: Readonly<string> | ||
| mutate: KeyedMutator<any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider specifying a more precise type for KeyedMutator<any>. Using any can lead to potential type safety issues. If possible, replace any with a more specific type that matches the expected data structure.
| taxForm: { | ||
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific type for primaryCurrency if possible, such as an enum or a union of string literals representing valid currency codes, to ensure type safety.
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The estimatedFees field is currently typed as string | null. If this field represents a numeric value, consider using a numeric type instead to prevent potential issues with string manipulation.
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; | ||
| taxWithholdingPercentage?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The taxWithholdingPercentage field is typed as string | null. If this field is meant to represent a percentage value, consider using a numeric type to facilitate calculations and prevent errors related to string handling.
| winningsIds, | ||
| }) | ||
| const url = `${baseUrl}/withdraw` | ||
| const url = `${WALLET_API_BASE_URL}/withdraw` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider verifying that WALLET_API_BASE_URL is defined and has the correct value before using it to construct the URL. This will help prevent potential runtime errors if the environment variable is not set or is incorrect.
| }) | ||
|
|
||
| const url = `${baseUrl}/otp/resend` | ||
| const url = `${WALLET_API_BASE_URL}/otp/resend` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from baseUrl to WALLET_API_BASE_URL should be verified to ensure that WALLET_API_BASE_URL is correctly defined and imported in the module. This change could potentially affect the endpoint being called.
| * @param value - The input value which can be a string, `null`, or `undefined`. | ||
| * @returns The original value if it is a valid string (and not `'null'`), otherwise returns `'0'`. | ||
| */ | ||
| export const nullToZero = (value: string | null | undefined): string => (value === 'null' ? '0' : value ?? '0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using strict equality checks for null and undefined to ensure type safety. The current implementation uses the nullish coalescing operator (??) which is appropriate, but strict checks can be more explicit and clear.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?